-
-
Notifications
You must be signed in to change notification settings - Fork 831
Conversation
Signed-off-by: Jaiwanth <jaiwanth2011@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jaiwanth-v— thanks for the contribution! I have a couple of questions:
-
I'd like to get a better shared understanding of the context and motivation for this change, can you elaborate? Has there been common or multiple points of feedback that the previous blur setting is too leaky? If so, please can you add some links to this PR (GitHub issues, matrix.to links to matrix messages, etc).
-
The increase in blur size seems to be causing, or exaggerating a hard clip at the top or the bottom which doesn't meet our visual bar. Do you know what's causing it?
|
To add to the first question. I think text blurring was OK, but blurring for images was insufficient. See: element-hq/element-meta#1545 |
I also think the text blurring was OK, it actually looked roughly like words, so I could understand it to be hidden words. In the context of a timeline, I think that's important. The For the image spoiler not being blurred enough, I don't think it's solved by this. Someone showed me this the other week - using blurhash for media placeholders. I wonder if there's a way to use styling like this for the image spoiler? |
Yeah, that's exactly what I meant about images. That kind of blurring is insufficient. |
@niquewoodhouse Yes, using blurhash for rendering initial spoiler preview would be a great idea. For now, increasing the intensity of spoiler images looks good. The result would look something like this: |
The maxed out blur is for sure much better than before. Still not ideal though, since you can make out some shapes. |
It can be further increased. There's no upper bound. |
IMO increasing the blur intensity of all spoilers isn't the right way to approach the problem, since currently images can only be spoilered by inserting them inline in a textual message, which is somewhat of a hack. I expect that a spec change will be needed to properly support image spoilers, at which point it will hopefully be easy to handle images separately from text spoilers (e.g. by showing a blurhash) |
Actually, images inside spoilers can be targeted and can be given a different blur. That is what I did in this comment. That way we won't disturb the other text spoilers. |
Had a conversation about this today - we think image spoilers should be implemented using blurhash instead of this way. We would very happily accept PRs for that, but I now think this PR is irrelevant, so closing it. Thanks very much for your contribution! |
Fixes element-hq/element-meta#1545
Signed-off-by: Jaiwanth jaiwanth2011@gmail.com
Here's what your changelog entry will look like:
✨ Features